Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

chore(deps): Upgrade Zod to @latest #8762

Merged
merged 13 commits into from
Oct 30, 2023
Merged

chore(deps): Upgrade Zod to @latest #8762

merged 13 commits into from
Oct 30, 2023
+26 −11

Conversation

evadecker
Copy link
Contributor

@evadecker evadecker commented Oct 6, 2023 •
edited
Loading

Changes

Testing

  • No tests added

Docs

/cc @withastro/maintainers-docs for feedback!

@changeset-bot
Copy link

changeset-bot bot commented Oct 6, 2023 •
edited
Loading

🦋 Changeset detected

Latest commit: e668df3

The changes in this PR will be included in the next version bump.

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@github-actions github-actions bot added pkg: integration Related to any renderer integration (scope) pkg: astro Related to the core `astro` package (scope) labels Oct 6, 2023
natemoo-re
natemoo-re approved these changes Oct 6, 2023
View reviewed changes
Copy link
Member

@natemoo-re natemoo-re left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for the contribution!

@natemoo-re
Copy link
Member

Not sure what's going on with the CI failure, but it's definitely not your fault! We'll debug that.

@bluwy
Copy link
Member

bluwy commented Oct 10, 2023

Zod is specifically pinned because of that types issue. I spent quite some time on it before and it seems there's no way to workaround it (tsc just bails). The only way if we really want it to work is to unwrap this as a standalone type i think:

astro/packages/astro/src/core/config/schema.ts

Line 282 in 0ab6bad

export type AstroConfigType = z.infer<typeof AstroConfigSchema>;

@evadecker
Copy link
Contributor Author

@bluwy Thanks for that, I tried digging a little bit, too, but this level of TypeScript is a bit beyond me. Do you know why the pinned version of zod works but later releases don't?

@bumu
Copy link

bumu commented Oct 27, 2023

@evadecker can you help to fix the conflicts?

@github-actions github-actions bot added the pr: docs A PR that includes documentation for review label Oct 27, 2023
@evadecker
Copy link
Contributor Author

@evadecker can you help to fix the conflicts?

I reran pnpm i to get rid of the lockfile merge conflict, but we still have the issue that @bluwy mentioned.

@josephshambrook josephshambrook mentioned this pull request Oct 28, 2023
Closed
1 task
sarah11918
sarah11918 approved these changes Oct 28, 2023
View reviewed changes
Copy link
Member

@sarah11918 sarah11918 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just updating the verb tense of the changeset for consistency! Also, when just using the name of the thing, Zod appears to be capitalized, so did that too.

Docs will be happy with these changes! 🙌

.changeset/swift-rivers-impress.md Outdated Show resolved Hide resolved
@evadecker @sarah11918
Update .changeset/swift-rivers-impress.md
3c9a642 
Co-authored-by: Sarah Rainsberger <sarah@rainsberger.ca>
@evadecker evadecker changed the title chore(deps): Upgrade zod to @latest chore(deps): Upgrades Zod to @latest Oct 28, 2023
@evadecker evadecker changed the title chore(deps): Upgrades Zod to @latest chore(deps): Upgrade Zod to @latest Oct 28, 2023
@evadecker
Copy link
Contributor Author

evadecker commented Oct 28, 2023 •
edited
Loading

@bluwy @natemoo-re

I was able to resolve the previous error—

Error: src/core/config/schema.ts(62,14): error TS2742: The inferred type of 'AstroConfigSchema' cannot be named without a reference to '.pnpm/mdast-util-to-hast@12.3.0/node_modules/mdast-util-to-hast'. This is likely not portable. A type annotation is necessary.
Error: src/core/config/schema.ts(312,17): error TS2742: The inferred type of 'createRelativeSchema' cannot be named without a reference to '.pnpm/mdast-util-to-hast@12.3.0/node_modules/mdast-util-to-hast'. This is likely not portable. A type annotation is necessary.

—without unwrapping AstroConfigType as a standalone type. 🎉

To do this required explicitly adding mdast-util-to-hast as a devDependency (pinned @12.3.0), and importing mdast-util-to-hast in the config file.

The extra import feels odd to me, but it's the solution suggested by the TypeScript team developer lead when TypeScript's declaration emitter fails to resolve a reference.

@natemoo-re
Copy link
Member

@evadecker wow great job! I think that works. I'm curious if we'd need to add it as an actual dependency to ensure it's included in user's projects, though.

@evadecker
Copy link
Contributor Author

evadecker commented Oct 28, 2023 •
edited
Loading

Happy to make an edit if that's the case!

Not sure why macos-latest test failed: hopefully just a flaky test?

ematipico
ematipico approved these changes Oct 30, 2023
View reviewed changes
Copy link
Member

@ematipico ematipico left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@bluwy could you please take a look?

bluwy
bluwy reviewed Oct 30, 2023
View reviewed changes
Copy link
Member

@bluwy bluwy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One small request, but great work on fixing this! Looks like the move from shiki to shikiji allowed an easier fix here.

packages/astro/package.json Outdated Show resolved Hide resolved
natemoo-re
natemoo-re approved these changes Oct 30, 2023
View reviewed changes
Copy link
Member

@natemoo-re natemoo-re left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks so much for the PR @evadecker!

@natemoo-re natemoo-re merged commit 35cd810 into withastro:main Oct 30, 2023
13 checks passed
@astrobot-houston astrobot-houston mentioned this pull request Oct 30, 2023
Merged
@evadecker evadecker deleted the upgrade-zod branch October 30, 2023 21:04
natemoo-re added a commit that referenced this pull request Nov 22, 2023
@evadecker @sarah11918 @natemoo-re
chore(deps): Upgrade Zod to @latest (#8762)
9d42c6d 
Co-authored-by: Sarah Rainsberger <sarah@rainsberger.ca>
Co-authored-by: Nate Moore <nate@astro.build>
Co-authored-by: Nate Moore <natemoo-re@users.noreply.github.com>
@bluwy bluwy mentioned this pull request Feb 26, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bluwy bluwy bluwy left review comments

@natemoo-re natemoo-re natemoo-re approved these changes

@sarah11918 sarah11918 sarah11918 approved these changes

@ematipico ematipico ematipico approved these changes

Assignees
No one assigned
Labels
pkg: astro Related to the core `astro` package (scope) pkg: integration Related to any renderer integration (scope) pr: docs A PR that includes documentation for review
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

astro@3.4.0 and @astrojs/sitemap@3.0.2 using vulnerable version of Zod
6 participants
@evadecker @natemoo-re @bluwy @bumu @ematipico @sarah11918